fix(storage): reject path-traversal artifact ids at the path chokepoint by minion1227 · Pull Request #301 · vouchdev/vouch

minion1227 · 2026-07-01T03:49:12Z

What changed

Artifact ids are now rejected at the storage layer when they contain a path
separator, .., a nul byte, or are empty. A new _reject_unsafe_id guard
runs at the _yaml / _page_path chokepoint that every _*_path helper
funnels through, so all artifact read/write paths are covered at once.

Why

Source.id is locked to a hex sha256, but Claim, Page, Entity,
Relation, Evidence, Session, and Proposal all declare a bare
id: str with no validator, and _yaml turned that straight into
kb_dir/<sub>/<id>.yaml with no containment check. A poisoned id could then
escape kb_dir:

bundle smuggling → later write outside the tree. import_apply uses
_safe_member_path to keep each tar member's filename inside kb_dir,
but does not check the id field inside the YAML body. A claim landed
under claims/innocent.yaml whose id is "../../../tmp/pwned" reads
back fine (safe filename), but the next update_claim / supersede /
contradict resolves _claim_path(claim.id) to a path outside kb_dir.
arbitrary read on get. get_claim("../../../etc/hostname") resolved
straight through _yaml before this change.

This is the same class of hole the tar-member fix (bundle._safe_member_path,
the CVE-2007-4559 fix) already closed on the import path — _reject_unsafe_id
mirrors its _unsafe_name_reason checks at the storage chokepoint. closes #149.

What might break

nothing legitimate. artifact ids are flat slugs / hex / uuids — none contain
/, \, .., or nul — so every real read/write is unaffected (the full
suite, including bundle/sync/session round-trips, is green). the only calls
that now raise ValueError are ones that were already unsafe. no on-disk
format, kb.* method, or audit-log shape changes.

a complementary model-layer id validator (mirroring
Source._id_is_hex_sha256) is deliberately left as a follow-up: the storage
guard already closes every reach path, including raw-id reads a model
validator cannot see, and keeping it storage-only avoids overlapping the
model classes.

VEP

not a surface change — a storage-layer containment guard in the same shape
already merged for tar members. no VEP needed.

Tests

make check — ruff clean; mypy clean on touched files; pytest green
(only pre-existing, environment-only failures remain: fastapi/[web]
not installed and embeddings installed, both of which also fail on
main)
new tests in tests/test_storage.py: _yaml / _page_path reject
traversal / separator / absolute / empty ids (parametrised); put_*,
get_*, and put_page reject traversal ids; the disk-smuggled-id →
update_claim exploit chain is blocked before any write; and a
regression guard that legitimate slug / hex ids still resolve
CHANGELOG.md updated under ## [Unreleased]

Summary by CodeRabbit

Bug Fixes
- Strengthened storage handling to reject unsafe artifact and source IDs (including empty values, NUL bytes, path separators, and traversal segments) to prevent path traversal and unintended reads/writes outside the app data directory.
- Added model-layer validation to reject empty or whitespace-only text for claims, entity names, and page titles.
- Improved regression coverage, including a “disk smuggling” scenario, to ensure malicious on-disk content can’t be promoted via updates, while valid IDs still resolve normally.

`Source.id` is locked to a hex sha256, but `Claim`, `Page`, `Entity`, `Relation`, `Evidence`, `Session`, and `Proposal` all carry a bare `id: str` with no validator. `_yaml` / `_page_path` turned those ids straight into filesystem paths with no containment check, so a poisoned id — e.g. `"../../tmp/x"` smuggled through a bundle body that `import_apply` lands under a safe filename but whose in-memory id is traversal — could escape `kb_dir` on the next put / update / lifecycle write, or read an arbitrary file on get. add `_reject_unsafe_id`, mirroring `bundle._unsafe_name_reason`: an id must be non-empty and free of nul bytes, path separators, and `..` components. route it through `_yaml` and `_page_path` — the single chokepoint every `_*_path` helper funnels through — so all artifact read/write paths are guarded at once. a complementary model-layer id validator (mirroring `Source._id_is_hex_sha256`) is left as a follow-up; the storage guard already closes every reach path, including raw-id reads a model validator cannot see. closes vouchdev#149

coderabbitai · 2026-07-01T03:49:28Z

Walkthrough

Adds centralized storage-layer validation for unsafe artifact IDs, applies it to YAML, page, and source path construction, expands regression coverage for rejected and valid IDs, and documents the hardening in the changelog.

Changes

Artifact ID Path Traversal Guard

Layer / File(s)	Summary
Unsafe ID validator and path helper wiring `src/vouch/storage.py`	Adds `_reject_unsafe_id(obj_id)` and calls it from `_yaml(sub, obj_id)`, `_page_path(page_id)`, and `_source_dir(source_id)` before filesystem paths are built.
Regression tests and changelog `tests/test_storage.py`, `CHANGELOG.md`	Adds storage tests for unsafe-ID rejection across source, YAML, page, claim, and update flows, includes a disk-smuggling regression and safe-ID path checks, and documents the storage and model validation fixes. Note that the supplied code changes only include the changelog entry for the model validation item.

Estimated code review effort: 2 (Simple) | ~12 minutes

Possibly related PRs

vouchdev/vouch#300: Adds overlapping model validation for empty or whitespace-only Claim.text, Entity.name, and Page.title.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The changelog includes an unrelated model-layer validation fix for Claim.text, Entity.name, and Page.title that is outside issue `#149`.	Split the model-layer validation into a separate PR or remove it from this change set if it is not part of the linked issue.
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the main change: rejecting unsafe artifact IDs to prevent path traversal.
Linked Issues check	✅ Passed	The storage guard and regression tests cover the issue's core requirement: unsafe artifact IDs are rejected across path-building helpers.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/vouch/storage.py`:
- Around line 257-265: The storage path validation is incomplete because source
paths are still built from untrusted source IDs without rejection. Update the
source lookup flow in Storage, especially get_source(), read_source_content(),
and _source_dir(), to apply _reject_unsafe_id() to source_id before constructing
paths so traversal inputs cannot escape kb_dir. Keep the fix aligned with the
existing _yaml(), _page_path(), and _claim_path() validation pattern.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 868a3b3e-60b5-42f6-bee6-d17f55afff2f

📥 Commits

Reviewing files that changed from the base of the PR and between 94e84b1 and 137b515.

📒 Files selected for processing (3)

CHANGELOG.md
src/vouch/storage.py
tests/test_storage.py

address review on vouchdev#301: `_yaml` / `_page_path` were guarded, but `get_source` / `read_source_content` (and the evidence-ref existence checks) route raw `source_id` strings through `_source_dir` without it, so `get_source("../../outside")` could still escape kb_dir to `<outside>/meta.yaml` / `content`. `Source.id` is hex-locked on the model but these read paths take unvalidated strings. apply `_reject_unsafe_id` at the `_source_dir` chokepoint too, closing the last storage traversal path. add get_source / read_source_content rejection tests.

minion1227 · 2026-07-01T04:02:17Z

good catch — fixed in a5db33c. added _reject_unsafe_id at the _source_dir
chokepoint too, so get_source / read_source_content and the evidence-ref
existence checks reject traversal ids as well. Source.id is hex-locked on
the model, but those read paths take raw strings, so this closes the last
storage traversal path. added get_source / read_source_content rejection
tests; full suite still green.

plind-junior · 2026-07-06T07:53:33Z

@copilot resolve the merge conflicts in this pull request

resolves the storage.py conflict from vouchdev#301 after main advanced to 1.2.1. - storage.py: keep both additions at the same spot — the vouchdev#149 `_reject_unsafe_id` path-traversal guard (this branch) and main's `_log` / `_load_or_skip` corrupt-file resilience helpers. the guard's three call sites in the `_*_path` helpers survived the merge intact. - changelog: move the vouchdev#149 entry into `[Unreleased]`; main had already cut the section it originally sat under into [1.0.0].

resolves the vouchdev#301 conflicts against its actual base branch (test, not main) after test advanced to vouchdev#345. - storage.py: keep both additions at the same spot — the vouchdev#149 `_reject_unsafe_id` path-traversal guard (this branch) and test's `_log` / `_load_or_skip` corrupt-file resilience helpers. the guard's three call sites in the `_*_path` helpers survived intact. - changelog / test_storage.py: keep both this branch's vouchdev#149 entries and the sibling vouchdev#155 empty-content entries test already carried.

…package the vouchdev#237 host-compat drift check read openclaw.compat.pluginApi from openclaw.plugin.json, but the 2026.6 loader repackage (47ac72f) moved that floor into the new loader-facing package.json — the current openclaw parser ignores openclaw.plugin.json's openclaw.* fields entirely. so _load_host_compat() read a key that no longer existed, host_compat silently degraded to {}, and the two drift tests failed on test. point _load_host_compat() and the drift tests at package.json, where the pluginApi floor now lives. the graceful-degradation and malformed-json tests move with it. no behaviour change beyond reading the right file.

# Conflicts: # CHANGELOG.md # src/vouch/capabilities.py # tests/test_capabilities.py

plind-junior · 2026-07-16T07:40:50Z

@coderabbitai review

coderabbitai · 2026-07-16T07:40:56Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 378-379: Move both fix entries currently under the 1.0.0 section
into the existing [Unreleased] ### Fixed section of CHANGELOG.md, preserving
their wording and ordering relative to the other unreleased fixes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 68c4d998-bb6d-41c9-b83f-556253679a66

📥 Commits

Reviewing files that changed from the base of the PR and between a5db33c and 7f2924e.

📒 Files selected for processing (2)

CHANGELOG.md
src/vouch/storage.py

🚧 Files skipped from review as they are similar to previous changes (1)

src/vouch/storage.py

coderabbitai · 2026-07-16T07:43:21Z

+- artifact ids that contain a path separator, `..`, a nul byte, or are empty are now rejected at the storage layer, closing a path-traversal hole. `Source.id` is hex-locked, but the other models (`Claim`, `Page`, `Entity`, `Relation`, `Evidence`, `Session`, `Proposal`) carry a bare `id: str`, so a poisoned id — e.g. one smuggled through a bundle body under a safe on-disk filename — could resolve to a path outside `kb_dir` on the next put / update / lifecycle write (or read an arbitrary file on get). The guard sits at the `_yaml` / `_page_path` chokepoint every `_*_path` helper funnels through, mirroring the `bundle._safe_member_path` tar-traversal fix (fixes #149).
+- `Claim.text`, `Entity.name`, and `Page.title` now reject empty / whitespace-only values at the model layer via `@field_validator`s, mirroring the `Claim.evidence` min-citation validator (#81/#82). The non-empty contract previously lived only in the `propose_*` helpers, so `store.put_*`, `store.update_*`, and bundle/sync import all silently accepted blank-content artifacts; enforcing on the model closes every write path at once (fixes #155).


📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Move these entries under [Unreleased].

Both new fixes are currently listed under ## [1.0.0], so they will be attributed to an already released version instead of appearing in the next release notes. Move Lines 378-379 into the [Unreleased] ### Fixed section.

As per coding guidelines, user-visible features must be documented under [Unreleased] in the same PR.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 378 - 379, Move both fix entries currently under the 1.0.0 section into the existing [Unreleased] ### Fixed section of CHANGELOG.md, preserving their wording and ordering relative to the other unreleased fixes.

Source: Coding guidelines

github-actions Bot added docs documentation, specs, examples, and repo guidance storage kb storage, migrations, schemas, and proposals tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 1, 2026

coderabbitai Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread src/vouch/storage.py

plind-junior changed the base branch from main to test July 6, 2026 07:53

minion1227 force-pushed the minion_149 branch from b48096b to 27465bb Compare July 6, 2026 08:53

minion1227 added 2 commits July 6, 2026 02:32

Merge remote-tracking branch 'upstream/test' into minion_149

a0681b5

github-actions Bot added the mcp mcp, jsonl, and http surfaces label Jul 6, 2026

minion1227 mentioned this pull request Jul 6, 2026

fix(capabilities): read host_compat from package.json after 2026.6 repackage #401

Open

4 tasks

Merge remote-tracking branch 'upstream/test' into minion_149

7f2924e

# Conflicts: # CHANGELOG.md # src/vouch/capabilities.py # tests/test_capabilities.py

github-actions Bot removed the mcp mcp, jsonl, and http surfaces label Jul 13, 2026

coderabbitai Bot reviewed Jul 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(storage): reject path-traversal artifact ids at the path chokepoint#301

fix(storage): reject path-traversal artifact ids at the path chokepoint#301
minion1227 wants to merge 6 commits into
vouchdev:testfrom
minion1227:minion_149

minion1227 commented Jul 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 1, 2026 •

edited

Loading

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

minion1227 commented Jul 1, 2026

Uh oh!

plind-junior commented Jul 6, 2026

Uh oh!

plind-junior commented Jul 16, 2026

Uh oh!

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		- artifact ids that contain a path separator, `..`, a nul byte, or are empty are now rejected at the storage layer, closing a path-traversal hole. `Source.id` is hex-locked, but the other models (`Claim`, `Page`, `Entity`, `Relation`, `Evidence`, `Session`, `Proposal`) carry a bare `id: str`, so a poisoned id — e.g. one smuggled through a bundle body under a safe on-disk filename — could resolve to a path outside `kb_dir` on the next put / update / lifecycle write (or read an arbitrary file on get). The guard sits at the `_yaml` / `_page_path` chokepoint every `_*_path` helper funnels through, mirroring the `bundle._safe_member_path` tar-traversal fix (fixes #149).
		- `Claim.text`, `Entity.name`, and `Page.title` now reject empty / whitespace-only values at the model layer via `@field_validator`s, mirroring the `Claim.evidence` min-citation validator (#81/#82). The non-empty contract previously lived only in the `propose_` helpers, so `store.put_`, `store.update_*`, and bundle/sync import all silently accepted blank-content artifacts; enforcing on the model closes every write path at once (fixes #155).

Uh oh!

Conversation

minion1227 commented Jul 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Why

What might break

VEP

Tests

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minion1227 commented Jul 1, 2026

Uh oh!

plind-junior commented Jul 6, 2026

Uh oh!

plind-junior commented Jul 16, 2026

Uh oh!

coderabbitai Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

minion1227 commented Jul 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 1, 2026 •

edited

Loading

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading